Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure authz role for API key is named after owner role #59041

Conversation

albertzaharovits
Copy link
Contributor

The composite role that is used for authz, following the authn with an API key, is an intersection of the privileges from the owner role and the key privileges defined when the key has been created.
This change ensures that the #names property of such a role equals the names of the owner role.

Relates #58928 (comment)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Authorization)

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jul 5, 2020
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good. Just one comment to discuss.

@@ -86,7 +81,7 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice
@Override
public Automaton allowedActionsMatcher(String index) {
final Automaton allowedMatcher = super.allowedActionsMatcher(index);
final Automaton limitedByMatcher = super.allowedActionsMatcher(index);
final Automaton limitedByMatcher = limitedBy.allowedActionsMatcher(index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gosh! Is this a bug!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. At first sight testing for it was not trivial. The bug doesn't seem important though. I'll see what I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a simple unit test 9738963, it is trivial after all.

The bug is not consequential. It could manifest when resizing and when adding aliases, when using keys with role descriptors, where it fails to allow the operation, although it should.

@tvernum
Copy link
Contributor

tvernum commented Jul 6, 2020

Per #58928 (comment), @albertzaharovits and I had a chat about this offline and I want to summarise why I think this change makes sense.

When we started thinking about API Keys, we planned to store them with a single list of privileges. That is, this API Key can do these things (e.g. write to a specific index). We intended that the create API Key endpoint would enforce that the privileges specified for the API Key would be a subset of the calling user's privileges.

However, there are edge cases that mean that we cannot universally determine whether an API Key is a subset of a user's privileges (or alternatively, perfectly calculate the intersection of the caller's privileges with the specified privileges).

So, instead the implement requires a runtime check that against the API key's privileges and the caller's privileges.

That design choice matters here because it has changed the way we think about and describe API Keys. We no longer think about API Keys as a thing that has its own privilege set, so much as a thing that has its owner's privilege set, but with an optional restriction (this is reflected in the API, where it is possible to create an API key that has no restrictions and simply inherits its owner's privileges).

The choice here, to swap the enclosing order in the LimitedRole, changes the implementation to reflect that reality - a API Key "Role" (implemented via LimitedRole) is the owner's privileges, with an optional set of restrictions.

@@ -65,6 +83,17 @@ public static void assertThrowsAuthorizationException(LuceneTestCase.ThrowingRun
assertAuthorizationException(securityException, messageMatcher);
}

public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyService, Authentication authentication,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something doesn't feel quite right about this.

Let's chat, but I'd prefer a public static method on the ApiKeyService (like createKeyForTesting) rather than making existing methods public.

Copy link
Contributor Author

@albertzaharovits albertzaharovits Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've been ignorant to break the ApiKeyService abstraction for test purposes.

I've created a public static inner class ApiKeyServiceTests.Utils that translates an authentication for a user to an authentication with an API Key.

authenticatedBy = new RealmRef(ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, nodeName);
writeAuthToContext(new Authentication(user, authenticatedBy, null, Version.CURRENT,
Authentication.AuthenticationType.API_KEY, authResult.getMetadata()));
final Authentication authentication = apiKeyService.createApiKeyAuthentication(authResult, nodeName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to echo on Tim's comments on favoring protected methods over public for test purpose. But otherwise, this PR LGTM. Thanks Albert!

@albertzaharovits albertzaharovits requested a review from tvernum July 6, 2020 11:56
@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jul 6, 2020

@tvernum if you approve of the changes from 103609e , i'll mimic them on #58928 or just merge this into master and merge master into #58928.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@albertzaharovits
Copy link
Contributor Author

@elasticmachine update branch

@albertzaharovits albertzaharovits merged commit d40ab54 into elastic:master Jul 7, 2020
@albertzaharovits albertzaharovits deleted the audit_owner_role_names_for_api_key_authn branch July 7, 2020 10:32
albertzaharovits added a commit that referenced this pull request Jul 7, 2020
The composite role that is used for authz, following the authn with an API key,
is an intersection of the privileges from the owner role and the key privileges defined
when the key has been created.
This change ensures that the `#names` property of such a role equals the `#names`
property of the key owner role, thereby rectifying the value for the `user.roles`
audit event field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants